Repository navigation
Conversation
CI failed: All five CI error templates trace back to one root cause: TypeScript cannot resolve the new `@sonarsource/analyzer-commons-configurations/identifier-urls.json` import in `identifier-urls.ts`, which breaks the unit-test, build, bridge, and Knip/bbf jobs. The PR itself introduces that import, so the failures are change-related.OverviewFive unique error templates from 5 logs (commit 4281d21) all show the same TS2307 error at FailuresUnresolved identifier-urls.json import (TS2307) (confidence: high)
Bridge build cascade (Maven exec failure) (confidence: high)
Summary
Code Review 🚫 Blocked 0 closed / 4 findings🟡 Medium risk · The new S5332 URL parser and shared-inventory matcher may misclassify identifier URLs at edge cases if consumed by the rule. Adds 🚨 Cross-Repo: Commons npm package doesn't publish identifier-urls.json📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:18 📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.test.ts:19 Fix here and in Breaks in: This file imports Here: Once Commons publishes a release that contains identifier-urls.json, bump There: In sonar-analyzer-commons-configurations, generate identifier-urls.json (the domain-to-regex map) into the npm packaging dir, add it to In SonarSource/sonar-analyzer-commons - 💡 Quality: Tests never exercise the URL parsing or matching logic📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.test.ts:23-29 📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:31-41 Both tests depend on the inventory being empty. One asserts 💡 Quality: isKnownIdentifierUrl is never called by the S5332 rule📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:31 The PR title says S5332 now consumes the shared identifier inventory. But 💡 Cross-PR: Padded URLs match in the Java filter but not in SonarJS · SonarSource/sonar-analyzer-commons #590📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:20 📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:34 Breaks when sonar-analyzer-commons#590 is released with a populated inventory and the S5332 rule checks a URL string that has leading or trailing whitespace. How
Fix
Compared 🤖 Prompt for agentsVerification Rules 1/1 met Repos 1 inspected · PRs 1 inspectedFunctional validation No results Rules 1 passed 2 not applicable
Broader impact Repos 1 inspected PRs 1 inspectedCross-repo 1 repository inspected 1 open impact
Cross-PR 1 PR inspected 1 open impact
Auto-approval Not enabledFor more verification details, comment Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 5 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 4281d21 | Docs | View more details | Give us feedback! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| it('uses an empty identifier inventory', () => { | ||
| assert.deepEqual(identifierUrls, {}); | ||
| }); | ||
|
|
||
| it('does not match URLs when the inventory is empty', () => { | ||
| assert.equal(isKnownIdentifierUrl('http://identifier.invalid/schema'), false); | ||
| }); |
There was a problem hiding this comment.
💡 Quality: Tests never exercise the URL parsing or matching logic
Both tests depend on the inventory being empty. One asserts identifierUrls deep-equals {}, which will fail as soon as Commons ships any entry. The other only checks that an empty map returns false. So the authority/remainder regex, lowercasing only the authority, keeping the port, adding / before ?/# suffixes, and running the per-domain regex all go untested. A bug in any of them would pass. Drive isKnownIdentifierUrl with a fixture inventory (for example, factor the matcher to accept a Map<string, RegExp>) and test port, case, query, fragment and non-matching paths. Drop the assertion that the upstream data is empty.
Was this helpful? React with 👍 / 👎
| * CleartextProtocolFilter.isKnownIdentifier(), is not exposed through that package. | ||
| * SonarJS therefore performs URL parsing and regex matching here in Node.js using the shared data. | ||
| */ | ||
| export function isKnownIdentifierUrl(value: string): boolean { |
There was a problem hiding this comment.
💡 Quality: isKnownIdentifierUrl is never called by the S5332 rule
The PR title says S5332 now consumes the shared identifier inventory. But isKnownIdentifierUrl is only imported by its own test: rule.ts, rule.lib.ts and rule.aws.ts don't reference it. So the rule still reports http:// identifier URLs exactly as before and the change has no effect on behavior. Either call the matcher from the cleartext-protocol check in rule.lib.ts (with the AST-usage check the comment requires), or describe the PR as adding the helper only.
Was this helpful? React with 👍 / 👎
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
|
|
||
| import identifierUrls from '@sonarsource/analyzer-commons-configurations/identifier-urls.json' with { type: 'json' }; |
There was a problem hiding this comment.
🚨 Cross-Repo: Commons npm package doesn't publish identifier-urls.json
Fix here and in SonarSource/sonar-analyzer-commons
Breaks in: SonarSource/sonar-analyzer-commons - sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json - files
This file imports @sonarsource/analyzer-commons-configurations/identifier-urls.json. In SonarSource/sonar-analyzer-commons, the files list in sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json ships only README.md, secret-patterns.json and secret-exclusion-corpus.json. The 2.31.0-5284 tarball that SonarJS pins matches that: I downloaded it and it has no identifier-urls.json. So the module can't be resolved, and TypeScript build, bundling and the new test all fail. Commons has to generate and publish the inventory in a new npm release, and this repo has to move its pin to that release.
Here: Once Commons publishes a release that contains identifier-urls.json, bump @sonarsource/analyzer-commons-configurations in package.json and regenerate package-lock.json. Until then, keep this PR in draft so master doesn't break.
There: In sonar-analyzer-commons-configurations, generate identifier-urls.json (the domain-to-regex map) into the npm packaging dir, add it to files in src/main/packaging/npm/package.json, and publish a new version.
In SonarSource/sonar-analyzer-commons - sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json:
"files": [
"README.md",
"secret-patterns.json",
"secret-exclusion-corpus.json",
"identifier-urls.json"
]
Was this helpful? React with 👍 / 👎
|
|
||
| import identifierUrls from '@sonarsource/analyzer-commons-configurations/identifier-urls.json' with { type: 'json' }; | ||
|
|
||
| // The inventory is maintained in Analyzer Commons; keep the matching semantics aligned. |
There was a problem hiding this comment.
💡 Cross-PR: Padded URLs match in the Java filter but not in SonarJS · SonarSource/sonar-analyzer-commons #590
Breaks when sonar-analyzer-commons#590 is released with a populated inventory and the S5332 rule checks a URL string that has leading or trailing whitespace.
How
CleartextProtocolFilter.isKnownIdentifiercallsurl.strip()before matching (commons/src/main/java/org/sonarsource/analyzer/commons/appsec/CleartextProtocolFilter.java:244).isKnownIdentifierUrlruns/^http:\/\/.../idirectly onvaluewith no trim, so any surrounding whitespace makesexecreturn null (packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:34).- So the same padded identifier URL is excluded by Java analyzers but still reported by SonarJS, even though the comment at
identifier-urls.ts:20says the matching semantics are kept aligned.
Fix
- This PR: Trim
valuethe same way as Java'sstrip()before running the authority regex, and add a test for a padded URL. - sonar-analyzer-commons#590: Nothing needed. Optionally list the whitespace stripping in the README's consumer matching rules.
Compared 4281d21 with sonar-analyzer-commons#590 at c0c18f9
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Comment gitar unblock to override this block and allow merging.
Configure merge blocking · Maintainers can dismiss this review.
Summary by Gitar
identifier-urls.tsloadingidentifier-urls.jsonfrom Analyzer Commons and exportingisKnownIdentifierUrlfor S5332identifier-urls.test.tschecking the inventory is empty and that no URL matchesThis will update automatically on new commits.